fix(auth): add withCredentials and fix CORS to persist session cookies#464
Conversation
❌ Deploy Preview for github-spy failed.
|
|
Warning Review limit reached
More reviews will be available in 54 minutes and 35 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughLogin and Signup pages update their axios POST requests to include ChangesFrontend Credential Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thank you @anshul23102 for your contribution. Please make sure your PR follows https://github.com/GitMetricsLab/github_tracker/blob/main/CONTRIBUTING.md#-pull-request-guidelines
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/server.js (1)
26-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
express-sessioncookiesameSite/secureoptions for credentialed cross-origin requests.
backend/server.jsenables CORS withcredentials: true, and the frontend sendsaxiosrequests with{ withCredentials: true }, but theexpress-sessionconfig lackscookie.sameSite/cookie.secure. For cross-origin deployments, browsers will reject/drop the session cookie unlesssameSite: 'none'andsecure: trueare set.File: backend/server.js
Lines: 26-30app.use(session({ secret: process.env.SESSION_SECRET, resave: false, saveUninitialized: false, }));🍪 Proposed fix for cross-origin cookie support
app.use(session({ secret: process.env.SESSION_SECRET, resave: false, saveUninitialized: false, + cookie: { + secure: process.env.NODE_ENV === 'production', // HTTPS only in production + sameSite: process.env.NODE_ENV === 'production' ? 'none' : 'lax', + httpOnly: true, + }, }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/server.js` around lines 26 - 30, The session cookie config in the app.use(session(...)) call is missing cookie options required for cross-origin credentialed requests; update the session middleware (the app.use(session) invocation) to include a cookie object with sameSite: 'none' and secure: true, and ensure the app trusts proxies when running behind an HTTPS terminator by setting app.set('trust proxy', 1) or equivalent so secure cookies are sent; optionally guard secure by NODE_ENV if you need a different behavior in development.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/server.js`:
- Around line 26-30: The session cookie config in the app.use(session(...)) call
is missing cookie options required for cross-origin credentialed requests;
update the session middleware (the app.use(session) invocation) to include a
cookie object with sameSite: 'none' and secure: true, and ensure the app trusts
proxies when running behind an HTTPS terminator by setting app.set('trust
proxy', 1) or equivalent so secure cookies are sent; optionally guard secure by
NODE_ENV if you need a different behavior in development.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2edab9d-7137-4ead-a7e8-37874a13d922
📒 Files selected for processing (3)
backend/server.jssrc/pages/Login/Login.tsxsrc/pages/Signup/Signup.tsx
|
Hi @GitMetricsLab team, just checking in on this one. The fix addresses the session cookie issue where the browser was silently dropping |
Login.tsx and Signup.tsx were sending axios POST requests without
{ withCredentials: true }, so the browser silently discarded the
Set-Cookie header on every cross-origin login/signup response. No
session cookie was ever stored, making every subsequent request
appear unauthenticated.
Changes:
- src/pages/Login/Login.tsx: pass { withCredentials: true } as the
third argument to axios.post for /api/auth/login
- src/pages/Signup/Signup.tsx: same fix for /api/auth/signup; also
remove the stale "Include cookies for session" comment that noted
the intent but was never fulfilled
- backend/server.js: replace cors('*') with a credentials-aware
config (origin: FRONTEND_URL, credentials: true); a wildcard origin
is rejected by browsers when credentials are present, so a specific
origin is required for Set-Cookie to be honoured
Fixes GitMetricsLab#414
anshul23102
force-pushed
the
fix/414-withcredentials-session-cookie
branch
from
fbf9cd8 to
d55fbcf
Compare
|
🎉🎉 Thank you for your contribution! Your PR #464 has been merged! 🎉🎉 |
2 participants
Summary
Fixes #414
The root cause of the broken session is a missing
{ withCredentials: true }in both axios calls, compounded by a wildcard CORS config that the browser rejects whenever credentials are involved. Three files are changed.Changes
src/pages/Login/Login.tsxAdded
{ withCredentials: true }as the third argument toaxios.postfor/api/auth/login. Without this flag the browser silently discards theSet-Cookieheader on every cross-origin response, so no session cookie is ever stored.src/pages/Signup/Signup.tsxSame fix for
/api/auth/signup. Also removed the stale inline comment// Include cookies for sessionthat documented the intended behaviour but was never implemented.backend/server.jsReplaced
cors('*')with a credentials-aware configuration:The Fetch specification forbids
Access-Control-Allow-Origin: *whenAccess-Control-Allow-Credentials: trueis set. Browsers reject the response silently, which meanswithCredentials: trueon the client side has no effect without this server-side change. A specific origin is required;FRONTEND_URLis read from the backend.envfile, with a localhost fallback for local development.Test plan
Cookieheader is present on subsequent requestsFRONTEND_URLinbackend/.envto the production URL and confirm CORS header matches in prodSummary by CodeRabbit